Use Poseidon2 hash function instead of BLAKE3 during the benchmark proving phase#3152
Use Poseidon2 hash function instead of BLAKE3 during the benchmark proving phase#3152Fumuran wants to merge 8 commits into
Poseidon2 hash function instead of BLAKE3 during the benchmark proving phase#3152Conversation
Fumuran
added
the
pr-from-maintainers
Is this using Falcon or ECDSA. If Falcon, let's switch this to ECDSA. |
Al-Kindi-0
left a comment
Al-Kindi-0
left a comment
There was a problem hiding this comment.
The time-counting proving benchmark only has auth coverage for single-P2ID consume. consume two P2ID notes is still Falcon-only and unlabeled, maybe worth it to add an ECDSA variant too.
Separately, the cycle-counting / bench-tx.json path still has create single P2ID note as Falcon-only and unlabeled.
| } | ||
|
|
||
| /// Creates a new [LocalTransactionProver] instance with the [`Poseidon2`] hash function. | ||
| pub fn with_poseidon2() -> Self { |
There was a problem hiding this comment.
Do we have to make this benchmark choice part of the public prover API?
The benchmark can already choose Poseidon2 with LocalTransactionProver::new(ProvingOptions::new(HashFunction::Poseidon2)), while Default keeps meaning the normal prover choice.
A new with_poseidon2() helper gives one hash function special API weight without a clear policy for Blake3 or future defaults.
There was a problem hiding this comment.
We can't use LocalTransactionProver::new(ProvingOptions::new(HashFunction::Poseidon2)) out of the box: HashFunction is part of the miden-core, but miden-core is not imported as a dependency in that crate.
So choosing between new dependency and verbose LocalTransactionProver::new(ProvingOptions::new(HashFunction::Poseidon2)) call vs no new dependency and convenience constructor I chose the latter.
In general it seems to me that it will be better to create such constructors for the other hash functions, which will allow us to avoid importing miden-core every time. I can create another constructor for Blake3 so the Poseidon2 won't be special anymore.
There was a problem hiding this comment.
We should probably change the default local prover to Poseidon2. Having Blake3 is actually misleading here as it makes the numbers look better than what they actually are.
So, I would update LocalTransactionProver::new() to instantiate the prover with Poseidon2 instead of introducing this new constructor.
This will probably affect proving times downstream cc @igamigo and @WiktorStarczewski.
There was a problem hiding this comment.
For now I'll explicitly implement Default for LocalTransactionProver to use Poseidon2, but in general we should update the default implementation of the ProvingOptions to use implicit implementation which is generated by #[derive(Default)] (we can't use it now because it is located in the miden-prover in the miden-vm repo). I'll create a tiny PR in the VM to update this, so after the next release we should be able to remove this explicit implementation and roll-back to the implicit one.
There was a problem hiding this comment.
I think this is fine because AFAICT, both client and node use LocalTransactionProver::default() to instantiate the prover. But would be good to double check to make sure we use Poseidon2 as the hash function everywhere. cc @Mirko-von-Leipzig @igamigo @WiktorStarczewski
Addresses PR #3152 review feedback: - Add ECDSA variants for every signature-authenticated benchmark (two P2ID, create P2ID) in both the cycle-counting and time-counting suites; single P2ID already had both. CLAIM/B2AGG authenticate the bridge account via network auth (no signature), so they keep a single variant. - Build the time-counting Criterion IDs programmatically so they encode the signing scheme and the proving hash function, e.g. 'Execute and prove transaction/poseidon2-falcon/single-p2id-note' and 'Execute and prove transaction/poseidon2/claim-note-l1'. - Regenerate bench-tx.json, refresh README examples, update CHANGELOG and the trace-contract test expectations.
Addresses PR #3152 review: instead of a Poseidon2-specific convenience constructor, change the prover default itself. Remove the with_poseidon2() and with_blake3() constructors and give LocalTransactionProver a manual Default impl that uses Poseidon2; the benchmark now proves via LocalTransactionProver::default().
….com/0xMiden/protocol into andrew-make-benchmarks-use-poseidon2
5 participants
Overview
Makes
Poseidon2the default proving hash function and improves the transaction benchmarks.Changes
LocalTransactionProverhash function fromBLAKE3toPoseidon2.Execute and prove transaction/poseidon2-falcon/single-p2id-noteand.../poseidon2/claim-note-l1.Notes
LocalTransactionProver::default()caller now proves withPoseidon2.Benchmark results
Here are the results of the "execute and prove" benchmarks:
So after switching from
Poseidon2toBLAKE3average "execute and prove" time increased by ~140%.Closes: #3129